Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Delete rows with matching unique key #478

Closed
wants to merge 3 commits into from
Closed

Delete rows with matching unique key #478

wants to merge 3 commits into from

Conversation

matthewrj
Copy link
Contributor

@matthewrj matthewrj commented Jan 17, 2022 •
edited
Loading

Description & motivation

This introduces "merge" behaviour for insert by period to match the standard incremental materialisation. This allows insert by period to be used with models where rows change over time.

I simply copied the code from the standard incremental materialisation here: https://github.com/dbt-labs/dbt-core/blob/8442fb66a591c4f5353f8baaaaf62f846c5f5171/core/dbt/include/global_project/macros/materializations/models/incremental/merge.sql#L50.

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
  • I followed guidelines to ensure that my changes will work on "non-core" adapters by:
    • dispatching any new macro(s) so non-core adapters can also use them (e.g. the star() source)
    • using the limit_zero() macro in place of the literal string: limit 0
    • using dbt_utils.type_* macros instead of explicit datatypes (e.g. dbt_utils.type_timestamp() instead of TIMESTAMP
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

@matthewrj matthewrj marked this pull request as ready for review January 17, 2022 21:32
@matthewrj matthewrj changed the base branch from main to next/minor January 17, 2022 22:00
@joellabes
Copy link
Contributor

@matthewrj Fancy! Thanks for opening this. It'd be nice to have this process support deletes 👍

Two conflicting thoughts here:

  1. I'd love to see some tests around this to ensure that it behaves correctly. I think this would be a series of seeds with different stages of progress, but I don't know how to cause it to re-run without multiple invocations of the automated tests, which feels a bit fragile...
  2. We're still thinking about the future of this macro (see Modernize legacy insert by period materialization #410), so I don't want to ask you to do a bunch of work that might go unmerged for a while.

For now, please don't be offended if this sits on ice for a bit - we're about to dive into the future of dbt-utils with a vengeance, so I expect there to be some progress in the next month or two.

@joellabes joellabes added pending and removed pending labels Jan 23, 2022
@joellabes
Copy link
Contributor

Hi @matthewrj, an update on this - have you seen #487? In short, this materialization will move into the experimental features repo in the next month or so. When that happens, we'll come back to this PR!

@vital-software vital-software closed this by deleting the head repository Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matthewrj @joellabes @vital-software